[HUDI-5341] CleanPlanner retains earliest commits must not be later than earliest pending commit #7568

SteNicholas · 2022-12-28T07:03:30Z

Change Logs

CleanPlanner should retain the earliest commits must not be later than the earliest pending commit. Meanwhile, HoodieTimelineArchiver should retain the clustering commit which instant can not be archived unless we ensure that the replaced files have been cleaned, without the replaced files metadata on the timeline, the fs view would expose duplicates for readers.

Impact

CleanPlanner retains the earliest commits must not be later than the earliest pending commit and HoodieTimelineArchiver retains the clustering commit which instant can not be archived unless we ensure that the replaced files have been cleaned

Risk level (write none, low medium or high below)

If medium or high, explain what verification was done to mitigate the risks.

Documentation Update

Describe any necessary documentation update if there is any new feature, config, or user-facing change

The config description must be updated if new configs are added or the default value of the configs are changed
Any new feature or user-facing change requires updating the Hudi website. Please create a Jira ticket, attach the
ticket number here and follow the instruction to make
changes to the website.

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

leesf · 2022-12-28T07:05:59Z

cc @stream2000

SteNicholas · 2022-12-28T08:26:20Z

@danny0405, @zhuanshenbsj1, I have created this pull request to fix the problem mentioned in #7405 in which the implementation is a little complex. PTAL.

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java

zhuanshenbsj1 · 2022-12-29T03:19:20Z

@danny0405, @zhuanshenbsj1, I have created this pull request to fix the problem mentioned in #7405 in which the implementation is a little complex. PTAL.

LGTM. Clean clustering instant sequentially，to keep no clean holes in timeline like archive seems much more easily to implement.

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java

SteNicholas · 2022-12-29T06:00:06Z

@leesf, could you please review this pull request? I have addressed above comments from @danny0405.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

SteNicholas · 2022-12-30T02:00:00Z

@yihua, could you please review this pull request? @leesf has approved this changes.

yihua · 2023-01-01T05:47:40Z

@SteNicholas Sorry for the delay. I'll review the PR soon.

SteNicholas · 2023-01-03T02:37:22Z

@yihua, any comments for this pull request?

yihua

The logic to determine the earliest instant to archive wrt clustering seems to be okay but it is very complex now. It would be good to think about how to simplify this as a whole.

yihua · 2023-01-03T19:55:46Z

HUDI-5493 for revisiting the logic.

yihua · 2023-01-04T01:34:28Z

@SteNicholas could you check the CI failure?

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java

…han earliest pending commit

SteNicholas · 2023-01-06T01:14:40Z

@yihua, I have already fixed the CI failure and updated the ClusteringUtils. Meanwhile the current failure has nothing to do with this change. PTAL.

SteNicholas · 2023-01-06T06:21:13Z

@hudi-bot run azure

hudi-bot · 2023-01-06T09:16:45Z

CI report:

d84940b Azure: FAILURE Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

…han earliest pending commit (#7568) (cherry picked from commit f745e64)

…han earliest pending commit (apache#7568)

nsivabalan · 2023-05-02T02:57:11Z

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java

    if (config.getCleanerPolicy() == HoodieCleaningPolicy.KEEP_LATEST_COMMITS
        && commitTimeline.countInstants() > commitsRetained) {
-      earliestCommitToRetain = commitTimeline.nthInstant(commitTimeline.countInstants() - commitsRetained); //15 instants total, 10 commits to retain, this gives 6th instant in the list
+      Option<HoodieInstant> earliestPendingCommits = hoodieTable.getMetaClient()


minor. singular for "earliestPendingCommits"

nsivabalan · 2023-05-02T03:02:13Z

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java

+        // Earliest commit to retain must not be later than the earliest pending commit
+        earliestCommitToRetain =
+            commitTimeline.nthInstant(commitTimeline.countInstants() - commitsRetained).map(nthInstant -> {
+              if (nthInstant.compareTo(earliestPendingCommits.get()) <= 0) {


We should use the timestamp compare apis in HoodieTimeline

HoodieTimeline.compareTimestamps(commit1Ts, GREATER_THAN, commit2Ts)

leesf self-assigned this Dec 28, 2022

danny0405 reviewed Dec 28, 2022

View reviewed changes

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java Show resolved Hide resolved

SteNicholas requested a review from danny0405 December 28, 2022 09:15

danny0405 reviewed Dec 28, 2022

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java Outdated Show resolved Hide resolved

SteNicholas requested a review from danny0405 December 28, 2022 09:39

yihua self-assigned this Dec 29, 2022

yihua self-requested a review December 29, 2022 02:38

yihua added priority:critical Production degraded; pipelines stalled area:table-service Table services labels Dec 29, 2022

danny0405 reviewed Dec 29, 2022

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java Show resolved Hide resolved

leesf reviewed Dec 29, 2022

View reviewed changes

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java Show resolved Hide resolved

leesf approved these changes Dec 29, 2022

View reviewed changes

yihua approved these changes Jan 3, 2023

View reviewed changes

boneanxs reviewed Jan 4, 2023

View reviewed changes

hudi-common/src/main/java/org/apache/hudi/common/util/ClusteringUtils.java Show resolved Hide resolved

[HUDI-5341] CleanPlanner retains earliest commits must not be later t…

d84940b

…han earliest pending commit

SteNicholas force-pushed the HUDI-5341 branch from cc53c8c to d84940b Compare January 5, 2023 13:21

SteNicholas requested a review from yihua January 5, 2023 13:54

SteNicholas mentioned this pull request Jan 6, 2023

[HUDI-5341] IncrementalCleaning consider clustering completed later #7405

Closed

4 tasks

leesf merged commit f745e64 into apache:master Jan 6, 2023

XuQianJin-Stars pushed a commit that referenced this pull request Feb 11, 2023

[HUDI-5341] CleanPlanner retains earliest commits must not be later t…

ee779fe

…han earliest pending commit (#7568) (cherry picked from commit f745e64)

nsivabalan pushed a commit to nsivabalan/hudi that referenced this pull request Mar 22, 2023

[HUDI-5341] CleanPlanner retains earliest commits must not be later t…

fe1ebaf

…han earliest pending commit (apache#7568)

fengjian428 pushed a commit to fengjian428/hudi that referenced this pull request Apr 5, 2023

[HUDI-5341] CleanPlanner retains earliest commits must not be later t…

e601359

…han earliest pending commit (apache#7568)

nsivabalan reviewed May 2, 2023

View reviewed changes

This was referenced Jul 20, 2023

[HUDI-6423] Incremental cleaning should consider inflight compaction instant #9038

Merged

[HUDI-6574] Fix the problem that incremental clean cannot be executed when the earliest ActiveTimeline is a pending commit. #9243

Closed

danny0405 mentioned this pull request Nov 17, 2023

[SUPPORT] Query failure due to replacecommit being archived #10107

Closed

hudi-bot mentioned this pull request Nov 30, 2025

Revisit the archival process wrt clustering #15663

Open

[HUDI-5341] CleanPlanner retains earliest commits must not be later than earliest pending commit #7568

[HUDI-5341] CleanPlanner retains earliest commits must not be later than earliest pending commit #7568

Uh oh!

Conversation

SteNicholas commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

Uh oh!

leesf commented Dec 28, 2022

Uh oh!

SteNicholas commented Dec 28, 2022

Uh oh!

Uh oh!

Uh oh!

zhuanshenbsj1 commented Dec 29, 2022

Uh oh!

Uh oh!

SteNicholas commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SteNicholas commented Dec 30, 2022

Uh oh!

yihua commented Jan 1, 2023

Uh oh!

SteNicholas commented Jan 3, 2023

Uh oh!

yihua left a comment

Choose a reason for hiding this comment

Uh oh!

yihua commented Jan 3, 2023

Uh oh!

yihua commented Jan 4, 2023

Uh oh!

Uh oh!

SteNicholas commented Jan 6, 2023

Uh oh!

SteNicholas commented Jan 6, 2023

Uh oh!

hudi-bot commented Jan 6, 2023

CI report:

Uh oh!

nsivabalan May 2, 2023

Choose a reason for hiding this comment

Uh oh!

nsivabalan May 2, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

SteNicholas commented Dec 28, 2022 •

edited

Loading

SteNicholas commented Dec 29, 2022 •

edited

Loading